feat: rework IPRateLimiter with Redis integration #4662
Conversation
Test Results 20 files ± 0 261 suites - 12 20m 48s ⏱️ - 1m 30s For more details on these failures, see this check. Results for commit 59f6388. ± Comparison against base commit 75f8196. This pull request removes 36 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
jasuwienas
left a comment
jasuwienas
left a comment
There was a problem hiding this comment.
@quiet-node It looks great overall! There’s just one thing I’m wondering about.
jasuwienas
left a comment
jasuwienas
left a comment
There was a problem hiding this comment.
lgtm @quiet-node
There’s one thing I dislike in our code (not introduced in this PR). Sometimes, when we create new interfaces, we prefix them with I (e.g., ICacheClient, IAccountInfo, IFeeHistory, and many others), and other times we don’t (e.g., the TxPool interfaces, global config, RateLimitStore, etc.). Maybe we should consider addressing this inconsistency later.
| this.logger = logger.child({ name: 'redis-rate-limit-store' }); | ||
| this.duration = duration; | ||
| this.rateLimitStoreFailureCounter = rateLimitStoreFailureCounter; | ||
|
|
There was a problem hiding this comment.
Amaziing, much cleaner!
There was a problem hiding this comment.
Great glad you find it better!
|
|
||
| describe('create', () => { | ||
| it('should return LruRateLimitStore when redisClient is not provided', () => { | ||
| const store = RateLimitStoreFactory.create(logger, testDuration); |
There was a problem hiding this comment.
Aren't not provided and undefined basically the same?
There was a problem hiding this comment.
Yeah I was thinking to not provided anything with undefined but with the counter. But you're right it's basically the same. I'll remove it
Yessir I definitely agree. Consistency makes everything so much more organized and tuitive! But yeah the I-prefixed-interfaces were the old ones and we're aiming to drop the I-prefix. That said we should have a new PR in future to align everything, even with the linting as well. |
quiet-node
dismissed stale reviews from simzzz, jasuwienas, and natanasow
via
c3b7928
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
quiet-node
force-pushed
the
4599-ratelimiter-refactor-storage-selection-to-use-factory-pattern
branch
from
c3b7928 to
59f6388
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4662 +/- ##
==========================================
- Coverage 95.67% 95.62% -0.05%
==========================================
Files 131 132 +1
Lines 21028 20989 -39
Branches 1785 1761 -24
==========================================
- Hits 20118 20071 -47
- Misses 892 903 +11
+ Partials 18 15 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Description
This PR:
RateLimitStoreFactoryto encapsulate the logic for selecting the storage implementation based on configuration.IPRateLimiterServiceto accept aRateLimitStoreinterface in its constructor.RedisRateLimitStoreto accept aRedisClientin its constructor instead of creating it internally.server.tsandwebSocketServer.tsto use the factory and inject the store.Related issue(s)
Fixes #4599
Changes from original design (optional)
RedisRateLimitStoreto accept aRedisClientin its constructor instead of creating it internally.Additional work needed (optional)
N/A
Checklist